JavaScript: Fix characteristic predicate of XMLParent.
#2520
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
I'm going to do a very quick dist-compare to ensure this doesn't cause optimiser wobbles. |
esbena
previously approved these changes
Dec 12, 2019
Contributor
esbena
left a comment
esbena
left a comment
There was a problem hiding this comment.
LGTM, with one comment to help our future selves.
| * both of which can contain other elements. | ||
| */ | ||
| class XMLParent extends @xmlparent { | ||
| XMLParent() { this instanceof @xmlelement or xmlEncoding(this, _) } |
Contributor
There was a problem hiding this comment.
Should we add a comment here or in the extractor about why we do it like this?
Contributor
Author
There was a problem hiding this comment.
Yeah, probably a good idea. Done.
The database type `@xmlparent` is defined a bit too loosely in that it includes all of `@file`, not just XML files. Fixing that would involve fiddling with the extractor/dbscheme, so I have opted to fix it at the QL level instead.
max-schaefer
force-pushed
the
js/fix-2517
branch
from
7574295 to
dfeca63
Compare
esbena
approved these changes
Dec 12, 2019
Contributor
Author
|
A smoke-test evaluation (internal link) showed no changes. I tried out DPM for this run, which worked like a charm (:tada:) and shows no changes to either tuple counts or predicted timings. I take that as evidence that join orders didn't change at all, so unless there is significant appetite for a more thorough evaluation I think this is good to go. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The database type
@xmlparentis defined a bit too loosely in that it includes all of@file, not just XML files. This meant that non-XML files were instances ofXMLParentbut not of any of its subtypes, thereby inheriting the defaultnone()-implementation ofgetName(), causing theirtoString()predicates to be empty.Fixing the extent of
@xmlparentwould involve fiddling with the extractor/dbscheme, while fixingXMLParentcan be done at the QL level, so that's what I've done instead.Fixes #2517.